-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: translate channel log #32612
fix: translate channel log #32612
Conversation
@cubuspl42 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
editAction: ({action}: EditActionParams) => `Edit ${action?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU ? 'request' : 'comment'}`, | ||
deleteAction: ({action}: DeleteActionParams) => `Delete ${action?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU ? 'request' : 'comment'}`, | ||
deleteConfirmation: ({action}: DeleteConfirmationParams) => `Are you sure you want to delete this ${action?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU ? 'request' : 'comment'}?`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, Currently the error happen because required cycle
. In en.ts
import ReportActionsUtils
and ReportActionsUtils.ts
we import Localize.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please force-push this branch, rewriting it in the folloiwng way...
This is not reviewable in this state |
294de20
to
35c409e
Compare
@cubuspl42 revert and add commit fix. |
Oh, so the circular import was the thing that caused the seemingly SVG-related error? |
Yeah I am not sure why, but it lead this error. |
Please include the relevant context in the "Details" section, for example: "A second attempt to fix (issue link), after it caused (regression issue link) and got reverted in (revert PR link). If somebody will ever need to navigate this graph (and this happens), they will be grateful. |
yeah, I updated detail |
We have a conflict, but it's not a random one. I think we're conflicting with the actual revert we want to revert. I'm sorry to ask, but would you rewrite the history again, but this time revert the revert using
On top of that, you can cherry-pick the commit with the new fix. Thank you for understanding. If that's easier for you, you can take these commits directly from here. I have fixed the conflict and cherry-picked your changes. Assuming you're on git remote add cubuspl42 https://github.com/cubuspl42/App.git
git fetch cubuspl42
git reset --hard cubuspl42/translate-channel-log-rewritten-1
git push --force |
This reverts commit 9d31e4b.
df8904d
to
09f457e
Compare
Thanks, I updated based on your suggestion and merged main. |
Reviewer Checklist
Screenshots/VideosWebtranslate-channel-log-again-web.mp4Mobile Web - Chrometranslate-channel-log-again-android-web-compressed.mp4Mobile Web - Safaritranslate-channel-log-again-ios-web.mp4Desktoptranslate-channel-log-again-desktop.mp4iOStranslate-channel-log-again-ios.mp4Androidtranslate-channel-log-again-android-compressed.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky one, thanks for sticking through it
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.12-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.12-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.12-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.12-2 🚀
|
.map((messageElement) => { | ||
switch (messageElement.kind) { | ||
case 'userMention': | ||
return `<mention-user accountID=${messageElement.accountID}></mention-user>`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@namhihi237 Why didn't we include the mentioned display name here? Did it cause any regression? For example:
`<mention-user accountID=${messageElement.accountID}>${messageElement.content}</mention-user>`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tienifr As I know currently mention-user use MentionUserRenderer
to display, in here the display name will get by accountID if we pass accountID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we use like you mention i think the ${messageElement.content}
does not effect because we pass the accountID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah thanks! I just want to make sure adding ${messageElement.content}
does not cause any issue.
I'm working on copying channel log function, I have two questions:
@namhihi237 Please support me. Thanks! |
@tienifr In This scope of PR we only support isMemberChangeAction |
Details
A second attempt to fix #30087, after it caused regression and got reverted in #32601
Fixed Issues
$ #30087
PROPOSAL: #30087 (comment)
Tests
Offline tests
N/A
QA Steps
The same test
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2023-12-07.at.09.37.01.mov
Android: mWeb Chrome
Screen.Recording.2023-12-07.at.09.43.26.mov
iOS: Native
Screen.Recording.2023-12-07.at.09.48.56.mov
iOS: mWeb Safari
Screen.Recording.2023-12-07.at.09.49.33.mov
MacOS: Chrome / Safari
Screen.Recording.2023-12-07.at.09.42.20.mov
MacOS: Desktop
Screen.Recording.2023-12-07.at.09.51.38.mov